-
Notifications
You must be signed in to change notification settings - Fork 141
Implement Applicative based API #522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about for
/for_
/traverse_
etc?
Good point! I forgot about them. But they're simpler since in this case one isn't in business of constructing vector so it simply reduces to fold. |
I think that's about as far as reasonably possible for improving performance. For many |
generateA is used as primitive and all other functions are expressed in it terms. First version goes through intermediate list. This is simplest implementation possible and would serve as baseline for further optimizations
We establish implementation which goes through list as baseline and the we can try to optimize it. Note definition of foldlOf'. It's different from definition in lens<=5.3.3 but it's absolutely necessary to get good perfomance in folds
Does wonders for traversals using Identity
Co-authored-by: konsumlamm <[email protected]>
No performance change in benchmarks
This is clearly not enough. There're many other types that will benefit form same rewrite but we don't know how to do that. unstreamM suffers from same problem. Identity is important since it's used in lens for mapping (over) and rewrite rule does improve performance: 10-20% in microbenchmark.
Relevant for TypeApplication and other function library use same convention
Now it works in the same way as generateM
I think PR is ready. |
It may perform better for some applicatives
We definitely can write such rewrite rules for specific cases. |
And for IO/ST there're such rules already. But it seems there's no way to write such rules compositionally. If for example we want to add rewrite rules for WriterT/ReaderT/StateT over IO then there're exponentially many types for given stack depth. We probably need some variant of typeclass guided rewrites |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not reviewing it earlier. Very busy time for me right now.
Looks great. I only had a few minor comment that don't necessarily need to be addressed
@@ -266,7 +266,11 @@ xs // ps = go xs ps' 0 | |||
go [] _ _ = [] | |||
|
|||
|
|||
withIndexFirst m f = m (uncurry f) . zip [0..] | |||
-- withIndexFirst :: (Int -> a -> [a]) -> [a] -> [a] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot a comment
-- withIndexFirst :: (Int -> a -> [a]) -> [a] -> [a] |
|
||
* [#522](https://github.com/haskell/vector/pull/522) API using Applicatives | ||
added: `traverse` & friends. | ||
* [#518](https://github.com/haskell/vector/pull/518) `UnboxViaStorable` added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an accidental indentation
* [#518](https://github.com/haskell/vector/pull/518) `UnboxViaStorable` added. | |
* [#518](https://github.com/haskell/vector/pull/518) `UnboxViaStorable` added. |
-- Use fromListN to be more efficient in construction of resulting vector | ||
-- Also behaves better with compact regions, preventing runtime exceptions | ||
in Data.Vector.fromListN n Applicative.<$> Traversable.traverse f (toList xs) | ||
traverse = traverse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have used the imported generic version instead for clarity. Otherwise at first glance it looks like bottom because of a recursive call. It is only when one looks at a qualified import for Traversable
it becomes clear that it is not the case.
traverse = traverse | |
traverse = G.traverse |
No problem. I you want to make a review but don't have time at the moment please leave note. I would wait |
Implementation follows plan described in #477 with
generateA :: Applicative f => Int -> (Int -> f a) -> f (v a)
as primitive. It is not subject to stream fusion.Writing
generateA
is not difficult. Problem is doing it efficiently. Current benchmarks are (suggestions for more are very welcome):First and naive implementation uses list as intermediate data structure. Sum benchmark performs well in this case. Using
STA
instead brings map benchmark on par with explicit loop and produces slight (5-10%) improvements in state and IO benchmark)Currently sum and map perform on par with explicit loop. State gives 7x slowdown and 8x allocations, IO benchmark 4x slowdown and 8x allocations. We obviously can add rewrite rules for
IO
/ST
but maybe there're more general optimizations.Fixes #477, #69, #132, #144